Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing inkscape package from the minimal-notebook #1765

Merged
merged 22 commits into from
Aug 20, 2022
Merged

Removing inkscape package from the minimal-notebook #1765

merged 22 commits into from
Aug 20, 2022

Conversation

Bidek56
Copy link
Contributor

@Bidek56 Bidek56 commented Aug 8, 2022

Describe your changes

Removing inkscape package from the minimal-notebook as suggested by @benz0li in this issue.

Checklist (especially for first-time contributors)

  • [x ] I have performed a self-review of my code
  • [ x] If it is a core feature, I have added thorough tests
  • [x ] I will try not to use force-push to make the review process easier for reviewers
  • [ x] I have updated the documentation for significant changes

@benz0li
Copy link
Contributor

benz0li commented Aug 9, 2022

@Bidek56 For the r-notebook and the datascience-notebook you need to create file /opt/conda/lib/R/etc/Rprofile.site and add either

options(jupyter.plot_mimetypes = c('text/plain', 'image/svg+xml', 'application/pdf'))

or

options(jupyter.plot_mimetypes = c('text/plain', 'image/png', 'image/jpeg', 'image/svg+xml', 'application/pdf'))

depending on how many different formats the notebook file (.ipynb) should include.

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 9, 2022

Any reason to use /opt/conda/lib/R/etc/Rprofile.site instead of ~/.Rprofile? Thx

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 9, 2022

I do not know much about R but based on this article we can set the plot_mimetypes right in the code.
Why would we want to set them globally for all users? Thx

@benz0li
Copy link
Contributor

benz0li commented Aug 9, 2022

Why would we want to set them globally for all users? Thx

Setting c.InlineBackend.figure_formats = {"png", "jpeg", "svg", "pdf"} in jupyter_server_config.py sets it globally for the Python kernel (IPython).

Setting options(jupyter.plot_mimetypes = c('text/plain', 'image/png', 'image/jpeg', 'image/svg+xml', 'application/pdf')) in /opt/conda/lib/R/etc/Rprofile.site sets it globally for the R kernel (IRkernel).

@benz0li
Copy link
Contributor

benz0li commented Aug 9, 2022

I am only using c.InlineBackend.figure_formats = {"svg", "pdf"} and options(jupyter.plot_mimetypes = c('text/plain', 'image/svg+xml', 'application/pdf')); no need for png and jpeg.

ℹ️ Every format specified is stored in the notebook file (.ipynb).

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 9, 2022

Why set these at all? Why not add a documentation line showing users how to add this line to their notebook: options(jupyter.plot_mimetypes = "image/svg+xml")

@benz0li
Copy link
Contributor

benz0li commented Aug 9, 2022

Why set these at all? Why not add a documentation line showing users how to add this line to their notebook: options(jupyter.plot_mimetypes = "image/svg+xml")

Removing inkscape without setting these globally will result in errors by default. That should not be the case.

It must work with the default settings.

Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't like copy-pasting the profile file. I suggest we add it to minimal-notebook.
    Because we change this behaviour for every user of minimal-notebook or image that depends on it, it makes more sense.

  2. Could we add a test that image rendering works properly for both Python and R notebooks?

datascience-notebook/Dockerfile Outdated Show resolved Hide resolved
@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 10, 2022

  1. I don't like copy-pasting the profile file. I suggest we add it to minimal-notebook.

Are you saying that you are OK with adding /opt/conda/lib/R/etc/Rprofile.site, ~/.Rprofile or neither? Thx

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 10, 2022

Why set these at all? Why not add a documentation line showing users how to add this line to their notebook: options(jupyter.plot_mimetypes = "image/svg+xml")

Removing inkscape without setting these globally will result in errors by default. That should not be the case.

It must work with the default settings.

This code works without jupyter.plot_mimetypes:

# Create a plot with some normally distributed data
library("ggplot2")
set.seed(42)
n <- 1000
p <- ggplot(data.frame(x = rnorm(n), y = rnorm(n)), aes(x=x, y=y)) + 
        geom_point(alpha = 0.25, size = 1, colour = "blue") + geom_density2d(colour = "red")

p + ggtitle(sprintf("Mime type = '%s'", getOption("jupyter.plot_mimetypes")))

since by default it seems to be set to image/png and application/pdf according to the docs
Can you please share a test case or code sample where it would fail without jupyter.plot_mimetypes? Thx

@mathbunnyru
Copy link
Member

  1. I don't like copy-pasting the profile file. I suggest we add it to minimal-notebook.

Are you saying that you are OK with adding /opt/conda/lib/R/etc/Rprofile.site, ~/.Rprofile or neither? Thx

Yes, I'm ok with both options. I prefer /opt/conda/ version, it is unlikely to mess with the user mounts.

Why set these at all? Why not add a documentation line showing users how to add this line to their notebook: options(jupyter.plot_mimetypes = "image/svg+xml")

Removing inkscape without setting these globally will result in errors by default. That should not be the case.
It must work with the default settings.

This code works without jupyter.plot_mimetypes:

# Create a plot with some normally distributed data
library("ggplot2")
set.seed(42)
n <- 1000
p <- ggplot(data.frame(x = rnorm(n), y = rnorm(n)), aes(x=x, y=y)) + 
        geom_point(alpha = 0.25, size = 1, colour = "blue") + geom_density2d(colour = "red")

p + ggtitle(sprintf("Mime type = '%s'", getOption("jupyter.plot_mimetypes")))

since by default it seems to be set to image/png and application/pdf according to the docs Can you please share a test case or code sample where it would fail without jupyter.plot_mimetypes? Thx

Unfortunately, I can't help with the example here.
@benz0li maybe you could provide one?

@benz0li
Copy link
Contributor

benz0li commented Aug 10, 2022

The default for IPython is definitively c.InlineBackend.figure_formats = {"png"} only – plus text/plain, the non-removable default for IPython.

Tested with jupyter/scipy-notebook and

import numpy as np
import matplotlib.pyplot as plt

r = np.arange(0, 2, 0.01)
theta = 2 * np.pi * r
fig, ax = plt.subplots(
  subplot_kw = {'projection': 'polar'} 
)
ax.plot(theta, r)
ax.set_rticks([0.5, 1, 1.5, 2])
ax.grid(True)
plt.show()

output
ℹ️ Image size: 277 x 269 pixels, Image DPI: 72 pixels/inch
(Retina would be Image size: 555 x 538 pixels, Image DPI: 144 pixels/inch)

Notebook file: No application/pdf in data; "text/plain": ["<Figure size 432x288 with 1 Axes>"]

@benz0li
Copy link
Contributor

benz0li commented Aug 10, 2022

The default for IRkernel is jupyter.plot_mimetypes = c('text/plain', 'image/png') with jupyter.plot_scale = 2.

Tested with jupyter/r-notebook and

library(ggplot2)

ggplot(airquality, aes(Temp, Ozone)) + 
  geom_point() + 
  geom_smooth(method = "loess"
)

output

ℹ️ Image size: 840 x 840 pixels, Image DPI: 120 pixels/inch
(Due to jupyter.plot_scale = 2 displayed as 480 x 480 pixels, 240 pixels/inch [aka retina] in JupyterLab)

getOption("jupyter.plot_mimetypes")
'text/plain' 'image/png'
getOption("jupyter.plot_scale")
2

Notebook file: No application/pdf in data; "text/plain": ["plot without title"]

@benz0li
Copy link
Contributor

benz0li commented Aug 10, 2022

Hope this helps in understanding the current IPython and IRkernel defaults.

And as mentioned before: IJulia is a whole different story...

@mathbunnyru
Copy link
Member

What's the status of this PR?

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 17, 2022

What's the status of this PR?

I am trying to create tests as per your request but unfortunately I do not know enough about it.

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 17, 2022

@benz0li @mathbunnyru I have added basic R tests, unfortunately I do not know enough about ggplot2 to add more advance tests. Feel free to add more sophisticated tests or share them so I can add them. Thanks

@mathbunnyru
Copy link
Member

Could you please remove code duplication as well?

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 17, 2022

Which code duplication? In the tests?

@mathbunnyru
Copy link
Member

The test file, Rprofile.site config file and some lines in Dockerfiles.

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 17, 2022

They are used in different stacks, how would you suggest we re-use them across multiple stacks?

@mathbunnyru
Copy link
Member

They are used in different stacks, how would you suggest we re-use them across multiple stacks?

As we discussed above, for the config file, let's put it to minimal-notebook.
With the test file, just create a test file somewhere with all the code and then import then create two small files in the notebook test folders importing this file content. I think this will work well. Or, put the code in one of two test files and import it in another one.

@Bidek56
Copy link
Contributor Author

Bidek56 commented Aug 17, 2022

They are used in different stacks, how would you suggest we re-use them across multiple stacks?

As we discussed above, for the config file, let's put it to minimal-notebook. With the test file, just create a test file somewhere with all the code and then import then create two small files in the notebook test folders importing this file content. I think this will work well. Or, put the code in one of two test files and import it in another one.

The jupyter_server_config.py config file is in base-notebook, why would we move it to minimal?
Rprofile.site belongs to datascience-notebook and r-notebook so it does not make senses to move to move to minimal, what I am missing here? Thx

@mathbunnyru
Copy link
Member

mathbunnyru commented Aug 17, 2022

Sorry for not being clear.

I propose the following:

  1. jupyter_server_config.py stays in the same place - I do not propose to change it's location.
  2. Rprofile.site should be in minimal-notebook. There are 2 good reasons for this - code duplication and also that this change might break the R users who for whatever reason decided to have their images based on minimal-notebook. This way we will try not to break these users.
  3. minetypes_R.ipynb and test_R_notebook.py should not be duplicated as well - I described how we can avoid the duplication quite easily.

minimal-notebook/Dockerfile Outdated Show resolved Hide resolved
minimal-notebook/Rprofile.site Outdated Show resolved Hide resolved
minimal-notebook/Rprofile.site Outdated Show resolved Hide resolved
tests/R_minetype_check.py Outdated Show resolved Hide resolved
@benz0li
Copy link
Contributor

benz0li commented Aug 19, 2022

With the current changes, IPython and IRkernel are not in sync regarding figure formats/plot mimetypes.

Either reduce c.InlineBackend.figure_formats to {"svg", "pdf"} or extend jupyter.plot_mimetypes to c('text/plain', 'image/png', 'image/jpeg', 'image/svg+xml', 'application/pdf'). 👉 Either #1765 (comment) or #1765 (comment).

ℹ️ Every format specified is stored in the notebook file (.ipynb).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants